Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(ml): model sessions #10559

Merged
merged 4 commits into from
Jun 25, 2024
Merged

refactor(ml): model sessions #10559

merged 4 commits into from
Jun 25, 2024

Conversation

mertalev
Copy link
Contributor

Description

The ML service has some coupling with ONNX Runtime: much of the logic in the base inference class is only relevant to this backend, while handling for other backends is treated as a special case. This PR abstracts ONNX Runtime into a ModelSession wrapper similar to how ARM NN is handled.

The catalyst for this change is that facial recognition recently broke for ARM NN due to a change that only applied to ONNX Runtime. As part of this, it also adds relevant tests for facial recognition, as well as AnnSession in general as its test coverage was quite low.

How Has This Been Tested?

Tested that the endpoint still works for each model task using ONNX Runtime. I'm unable to test ARM NN.

@mertalev mertalev force-pushed the refactor/ml-model-session branch from aa65409 to 1eac415 Compare June 23, 2024 04:00
@fyfrey
Copy link
Contributor

fyfrey commented Jun 23, 2024

Great refactoring, I'll give this a spin on ARM in few days

Copy link
Contributor

@fyfrey fyfrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on my prod instance, ARM-NN face detection&recognition models work fine again with this change :)

@mertalev mertalev merged commit 6356c28 into main Jun 25, 2024
23 checks passed
@mertalev mertalev deleted the refactor/ml-model-session branch June 25, 2024 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants